Remove vals from BindingsArray#11642
Conversation
|
One or more of the the following people are requested to review this:
|
chriseclectic
left a comment
chriseclectic
left a comment
There was a problem hiding this comment.
Looks like a nice simplification. Couple of non-blocking questions
|
I forgot to mention before, but can we add the |
| self, | ||
| vals: ArrayLike | Iterable[ArrayLike] | None = None, | ||
| kwvals: Mapping[ParameterLike, Iterable[ParameterValueType]] | ArrayLike | None = None, | ||
| data: Mapping[ParameterLike, Iterable[ParameterValueType]] | ArrayLike | None = None, |
There was a problem hiding this comment.
Is ArrayLike allowed as data? The following case raises an error.
In [5]: ba = BindingsArray([])
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Cell In[5], line 1
----> 1 ba = BindingsArray([])
File ~/tasks/4_2024/qiskit/terra/qiskit/primitives/containers/bindings_array.py:98, in BindingsArray.__init__(self, data, shape)
92 self._data = {}
93 else:
94 self._data = {
95 _format_key((p,))
96 if isinstance(p, Parameter)
97 else _format_key(p): np.array(val, copy=False)
---> 98 for p, val in data.items()
99 }
101 self._shape = _infer_shape(self._data) if shape is None else shape_tuple(shape)
102 self._num_parameters = None
AttributeError: 'list' object has no attribute 'items'There was a problem hiding this comment.
@t-imamichi No, with this change its not. A zero param binding array would need to be initialized as BindingsArray(), BindingsArray(None) or BindingsArray({}). So i think the type here is wrong, the | ArrayLike should be removed
|
I made a patch of test_statevector_samplers to fix errors in CI. ihincks#1 |
|
It makes sense to simplify the data structure and I agree with the idea. vals = np.array(...)
# this PR
qc = RealAmplitudes(2, reps=2)
params = (param.name for param in qc.parameters)
sampler.run([(qc, {params: vals}], shots=100)
# currently
qc = RealAmplitudes(2, reps=2)
sampler.run([(qc, vals)], shots=100)
qc = QuantumCircuit(2, 2)
qc.h(0)
qc.cx(0, 1)
result = sampler.run([(qc, [(), (), ()]).result()
print(result[0].data)
# DataBin<3>(c=BitArray(<shape=(3,), num_shots=1024, num_bits=2>)) |
|
@t-imamichi I believe the coerce method of More than simplification, the reason for this is there were numerous ambiguities when mixing vals and kwvals in bindings array since some parameters are named, some arent, and some are assumed to have a fixed order, while others arent, which leads to issues when a pub is finally executed and values need to be assigned to a circuit. This meant there were basically two viable options: allow only vals or kwvals in a BindingsArray, but not both, or remove one of them. Removing one of them simplifies the internals quite a lot so I think the small inconvenience in this construction is probably worth it. |
ihincks
force-pushed
the
primitives/no-vals-bindings-array
branch
from
bfd7949 to
1990a8d
Compare
Pull Request Test Coverage Report for Build 7728484593
💛 - Coveralls |
| [[np.pi]], | ||
| np.array([np.pi]), | ||
| np.array([[np.pi]]), | ||
| [np.array([np.pi])], |
There was a problem hiding this comment.
This is a change in behaviour because the outer list of vals enabled (1,1) things to be honourary floats. I don't see any reason to keep that because the only reason (1,) shaped things are honourary floats is to have {("a",): [0]}. behave like {("a","b"): [0,1]}.
There was a problem hiding this comment.
It looks good.
Minor comment. I see the pattern BindingsArray({tuple(circuit.parameters): values}) often appears in the codes. It might be useful to introduce a utlity method such as BindingsArray.from_pair(circuit: QuantumCircuit, values: ArrayLike) -> BindingsArray. But it's just a one-line code and I understand Chris' comment #11642 (comment), so it's not a strong opinion.
|
Happy to add |
chriseclectic
left a comment
chriseclectic
left a comment
There was a problem hiding this comment.
I think the main remaining issue is the 1-parameter array edges cases:
for i, shape in enumerate([(), (1,), (3,), (1, 1), (3, 1)]):
try:
ba = BindingsArray({"a": np.zeros(shape)})
print(i, "Input array shape:", shape, ",\tas_array() array shape:", ba.as_array().shape)
except Exception as ex:
print(i, ex)raises exceptions for the last two cases:
0 Input array shape: () , as_array() array shape: (1,)
1 Input array shape: (1,) , as_array() array shape: (1, 1)
2 Input array shape: (3,) , as_array() array shape: (3, 1)
3 Could not unambiguously determine the intended shape, all shapes in {(1,), (1, 1)} are consistent with the input; specify shape manually.
4 Could not unambiguously determine the intended shape, all shapes in {(3, 1), (3,)} are consistent with the input; specify shape manually.
For a ndim > 1 input array with the last axis 1, we should probably default to treating it as the parameter axis unless a user specifies otherwise, since that is the shape that would be returned by as_array.
Im happy to merge this as is though and think about this some more for a followup PR
|
@chriseclectic I think b465b97 covers the cases you discussed. |
5 participants
Summary
In the spirit of having only one way to do things, this PR reduces the complexity of the
BindingsArrayclass to only support thekwvalscase, i.e. the case where parameter values are attached to their names. The other motivation for opening this PR is that we were running into confusing technical difficulties when trying to reason about bothvalsandkwvalsat the same time.Details and comments
Note that this change would not affect primitive user's ability to submit parameter values as a big numpy array, as seen in some of the examples. This is because
SamplerPubandEstimatorPubknow what the parameter names should be, and can therefore inject them if needed. This is also implemented in this PR.